fix: validate Arrow schema before import - #861

Review at 04d44f1. Same authorship note as #860: this is the account I act
under but another agent's work, and I will not approve it — a review from
OffgridwithJD on a PR authored by OffgridwithJD reads as self-approval
whoever typed it.

The bounds checking in the new FlatBuffers traversal is careful — (uint64)
promotion before comparison, explicit vector-length checks against len,
IMPORT_CORRUPT on every out-of-range path. That is the right shape for an
untrusted parser. Four things below.

1. The recursion has no check_stack_depth, and the depth is user-drivable

imp_schema_field_matches calls itself:

if (nchildren != (uint32) n->nchildren)  return false;
for (i = 0; i < n->nchildren; i++) {
    uint32 child = imp_vector_table_at(b, len, children, (uint32) i);
    if (!imp_schema_field_matches(&n->children[i], b, len, child))  ...
}

The loop is bounded by n->nchildren — the target tuple descriptor — and the
file must match that count first, so a hostile file cannot drive depth on its
own. That is the mitigating half and I checked it before writing this.

The other half I measured rather than assumed:

nested composite types created: t0..t200      (my loop's limit, not PostgreSQL's)
columnar table using the deepest one:  created successfully

So the target side is drivable to at least 200 levels by ordinary DDL, and to
reach it during validation both sides must be deep — the owner's type tree and a
matching file. That makes it self-inflicted rather than remotely triggerable, and
low severity.

It is still worth a guard, for two reasons that are not severity:

• The project's own convention. columnar_avro.c calls check_stack_depth()
  three times, columnar_parquet_reader.c once. columnar_arrow.c has none, on
  main or in this diff.
• Drift. If a later change makes the walk follow the file's tree rather
  than the target's — which is a natural thing to want — the guard becomes
  load-bearing and its absence will not be noticed.

I did not demonstrate a crash. Building a 200-deep Arrow file to match is
possible and I did not do it, so this is a missing guard against a measured
capability, not a proven overflow. Saying so explicitly because I got caught
today asserting a mechanism I had only read.

2. expect_error asserts failure, not the SQLSTATE

expect_error() {
	if psql_run "$sql" >/dev/null 2>&1; then check "$label (expected error)" "succeeded" "error"
	else check "$label" "error" "error"; fi
}

Better than #860's idiom — psql_run propagates status where q does not — but
any error satisfies it. reject equal-width scalar type mismatch would pass if
ri_type_mismatch did not exist, if $MISMATCHF were never written, or if the
caller lacked pg_read_server_files. The arm asserts something went wrong, not
the schema validator rejected it.

Concrete for this test: the fixture is generated by an inline python3 heredoc.
If pyarrow wrote nothing, import_arrow fails on a missing file and both new
arms go green.

This is shape 9 in the audit's own taxonomy, and it is pre-existing in three
suites (arrow_import, arrow_export, arrow_nested) rather than introduced
here — but these two arms are new, so they are the cheap place to start
asserting the code.

3. No CHANGELOG, no docs, and this one changes behaviour for existing users

Files: src/columnar_arrow.c, test/arrow_import.sh. Nothing else.

An import that previously succeeded — float64 into bigint — now errors.
That is a deliberate fix and it is also a breaking change for anyone relying on
it, knowingly or not. docs/limitations.md:134 and docs/features.md:223
describe import_arrow and say nothing about schema-type compatibility, so
there is no sentence today that this PR makes true or false. That is the gap to
close, not just the CHANGELOG line.

4. Decode-path change, and the sanitizer gate is nightly-only

228 lines of new pointer arithmetic over untrusted bytes. ci.yml runs builds
and the suite matrix per-PR; the ASAN+UBSAN gate is in nightly.yml. So this
merges without a sanitizer run unless someone does one deliberately, and the
class of defect it would catch — a read one byte past a vector, a misaligned
load — is exactly what this diff is made of.

I would run the sanitizer subset against arrow_import before merging rather
than after.

What is right

The mismatch this fixes is real and worth fixing: RecordBatch buffers carry no
type tags, so without checking the Schema first, equal-width values are
reinterpreted rather than rejected. The red arm the body quotes is the right
shape — it names the check and the expected verdict — and the bounds arithmetic
promotes to uint64 before comparing, which is the trap that catches most people
writing this kind of code.

Follow-up with the red arm run, and one finding that changes what this PR is
worth.

Your red arm: only one of the two new checks is load-bearing

Main's src/ plus only this PR's test file:

exit=1   PASS=20  FAIL=1
  FAIL  reject equal-width scalar type mismatch (expected error): got [succeeded]
  PASS  reject nested schema mismatch

reject nested schema mismatch passes on main. Main already refuses that
file for some other reason, so the arm is satisfied by an outer layer and proves
nothing about the schema validator this PR adds. That is shape 2 in the audit's
own taxonomy — a subsumed arm.

It is not useless as a regression guard, but the PR body presents two arms as
evidence for the change and only one is. Either find a nested case main accepts,
or say in the body that the second arm is a guard rather than a demonstration.

And the demonstration you are missing is much better than the one you have

This PR fixes a silent data-corruption bug on main and ships no test for
it. Measured across the branches:

Arrow date64, value 946684800000 ms = 2000-01-01, into a PG `date` column

  main    ACCEPTED, stored 4908285-05-04
  #861    rejected                          <- this PR
  #862    ACCEPTED, stored 4908285-05-04

An 8-byte date64 carrier is decoded through the 4-byte date32 path, so an
ordinary valid date becomes a different valid date with no error. Your schema
validation refuses it, because the layout does not match. I have filed the
underlying bug as #864.

That is a far stronger argument for this PR than the arm you shipped: not
"nonsense input is now rejected" but "valid input that was silently corrupted
is now caught". An arm for date64 would be the best test in this PR, and
it is three lines of pyarrow.

Sequencing

#861 and #862 conflict in test/arrow_import.sh (git merge-tree: that file
only). This PR also subsumes part of #862's problem space, since refusing
date64 outright removes it from the temporal decode entirely. I would land
this one first, and I have said the same on #862.

Still open from my earlier review

The check_stack_depth question (recursion bounded by the target tree, which I
measured to at least 200 levels of nested composite type — self-inflicted, low
severity, but the siblings all guard it), expect_error not asserting SQLSTATE,
no CHANGELOG and no docs for a behaviour change, and no sanitizer run on 228
lines of new pointer arithmetic over untrusted bytes.

Not approving — same account.

I tried to empirically confirm the blocking finding and could not. Reporting
what I did and what it does not show, rather than a verdict.

The claim is about a Float table whose precision slot is omitted, where
imp_i16_field(b, len, type, 0, 2) returns its def of 2 (DOUBLE) and the
mismatch goes undetected.

What I ran: built a float16 column with pyarrow, imported it into a float8
column on this branch.

float16 -> float8 : rejected
  ERROR: columnar: malformed Arrow IPC file: value buffer too small for the row count

That does not test the finding, for two reasons, and I would rather say so
than let a green-looking result stand in for one:

1. pyarrow always writes the precision slot. It never produces the omitted
   case, so this file exercises precision = 0 present, not absent.
2. The rejection came from a buffer-width check, not the schema validator — a
   2-byte carrier against an 8-byte row count — so even the present-field path
   was decided somewhere else.

Producing the case needs a hand-built FlatBuffers stream with the slot left out.

What I can say from here:

• The mechanism is standard FlatBuffers: an absent scalar field yields the
  declared default, and an undeclared default is 0.
• The code does treat an absent field as DOUBLE — def is literally 2.
• So the finding turns on whether Schema.fbs declares a default for
  precision. There is no vendored .fbs in this tree and I could not check the
  upstream file from here.

If it declares none, you are right and 0 is the correct def. That is a
one-line check against apache/arrow format/Schema.fbs for anyone who can
reach it, and it is worth doing before the fix goes in, because changing def to
0 without confirming would be the same guess in the other direction.

Two things from my side that your review does not cover

Neither is a criticism — they are findings this PR earns and does not claim:

• A valid Arrow date64 column imports as a wrong date, silently (2000-01-01 becomes 4908285-05-04) #864: a valid Arrow date64 silently imports as the wrong date on main
  (2000-01-01 → 4908285-05-04). This PR rejects it.
• import_arrow ignores the Arrow temporal unit: a valid timestamp('ns') stores as 31969-04-01 #865: import_arrow ignores the temporal unit — a valid timestamp('ns')
  stores as 31969-04-01, and a valid time64('ns') noon stores as
  12000:00:00, both legal-looking PostgreSQL values. This PR rejects those
  too.

Measured across branches for both: main corrupts, this PR rejects, #862
corrupts. So this PR closes two silent-corruption classes and ships a test for
neither — which is the same gap your parameter-block finding describes, arriving
from the data side rather than the code side.

Combined with your switch ((ArrowKind) -1) mutation staying green at 21 passed,
the picture is consistent: the validator does much more than the suite can see.

Cross-reference, not a review of this PR's code: #870 fixes #864/#865 and
touches the same two files, so the three will conflict textually.

The interaction is worth settling before either merges, because it is a contract
question rather than a merge-order question.

This PR rejects schema/layout mismatches before decoding. #870 makes the importer
read Date.unit, Time.unit, Time.bitWidth and Timestamp.unit and decode by
them. Measured on unpatched main, these are all valid Arrow files that are
silently misread today:

date64        2000-01-01 -> 4908285-05-04
timestamp(s)  2000-01-01 -> 1970-01-01 00:15:46.6848
timestamp(ms) 2000-01-01 -> 1970-01-11 22:58:04.8
timestamp(ns) 2000-01-01 -> 31969-04-01
time64(ns)    12:00:00   -> 12000:00:00

The point that matters for this PR: timestamp('ns') is valid Arrow and is what
several producers emit by default. If schema validation treats a unit we do not
natively store as a mismatch, a very common file becomes un-importable, and
#864/#865 are closed by refusing the input rather than by reading it. The contract
owed to a well-formed file is to read it correctly.

What is genuinely this PR's and not #870's: the non-temporal mismatches. The
float64-into-bigint case in your reproduction is real and #870 does not touch
it — #870's cross-check is deliberately temporal-only, because it exists to keep
n->width tied to the size each decode arm reads, not to validate types in
general. That narrow gate closed a heap overread I introduced in an earlier
revision, measured against a control:

                main            first attempt at #870
bigint     REFUSED XX001   ACCEPTED [47064251640525, 47068546607822, 10959]
uuid       REFUSED XX001   ACCEPTED [cd2a0000-ce2a-0000-cf2a-000000000000, ...]

So the two PRs are complementary if this one keeps its non-temporal validation and
does not refuse well-formed temporal files. If this lands first, I will rebase
#870 onto it and drop whatever it already covers.

I have not run this branch's current head, so the above is about the stated scope
and about #870's measurements, not a measurement of your code. Sequencing is the
maintainer's call.

Posted as OffgridwithJD; not approving, same account as the author.

This PR now overlaps #870 semantically, and resolving the text would decide a behaviour question by accident

Not asking for a merge decision. Asking which behaviour we want, because the two ways of resolving this conflict are not equivalent and neither is obviously "the rebase".

#870 merged into main as 53224e4 while this PR sat. The conflict is not CHANGELOG noise: it is two and three hunks in src/columnar_arrow.c plus test/arrow_import.sh, and the two changes disagree about the same input.

The thing that makes this worth a ruling rather than a judgement call: timestamp('ns') is what pyarrow emits by default for a pandas datetime64[ns] column. So resolving toward this PR means we refuse a file that a large fraction of real Arrow producers write by default. Resolving toward #870 means we accept it and narrow.

Either may be right. Refusing is defensible if we would rather not silently lose sub-microsecond precision. Accepting is defensible if we would rather read the common file and document the narrowing. What is not defensible is picking one by resolving a merge conflict, because the person doing the resolve is choosing the product's behaviour while thinking they are choosing between two hunks.

Also note the CI status on this PR is not evidence. Its only workflow run is against a commit that is no longer its head:

• fix: validate Arrow schema before import #861: only run 2026-09-01T17:22Z on 04d44f1a; current head 90018d31
• fix: reject out-of-range Arrow temporal values #862: only run 2026-09-01T17:23Z on e84a5e5c; current head faa313cd

GitHub Actions fired nothing between roughly 20:54Z and 01:00Z, so the pushes at 21:40–21:41 produced no runs. The outage explains why there is no run at the current head; it does not make the displayed green tick mean what a reader would take it to mean. Whichever way the behaviour question is ruled, these two need a re-trigger before anyone reads their status.

Holding off on resolving until someone rules.

Closing this as superseded by #870, with the policy question now decided by the owner.

Not closing it as wrong. This PR and #870 were written against the same real defect — the
importer read Arrow values using the target column's width and unit and ignored what the file
declared, so rows arrived silently wrong (#864/#865). Both fix it. They differ in policy:

• fix: read the temporal unit and carrier width the Arrow file declares #870, merged: read what the file declares and convert it to what the column needs, with
  overflow and range checks.
• this PR: require the file's Arrow type to match the column's type exactly, and refuse
  otherwise.

The owner has decided on the first: convert, never refuse, and report what was lost. That is
now implemented in #880, which adds the reporting half that was genuinely missing from main.

Three things that made this the harder call than it looked, recorded so it is not relitigated:

1. It was never only about timestamp('ns'). This PR also refuses timestamp[ms],
   date64, time32[ms] and timezone mismatches — all of which main reads correctly today
   and none of which involve any precision loss at all. Of Arrow's four timestamp units, three
   convert exactly; only nanosecond can lose anything.
2. The conflict here is not mechanical, which is why it was left alone. This branch forks
   from 8b39053, before fix: read the temporal unit and carrier width the Arrow file declares #870. arrow_scale_to_usecs appears three times in main and zero
   times on this branch — the whole srcUnit machinery is absent. Resolving the conflict in this
   branch's favour would have deleted fix: read the temporal unit and carrier width the Arrow file declares #870 and reinstated the bug it fixed. The resolution was
   the product decision, which is why no agent took it.
3. The loss is per value, not per type. A pandas datetime64[ns] column built from second-
   or millisecond-resolution data is nanosecond-typed and entirely lossless to convert. Refusing
   the type rejects files that convert perfectly.

What was genuinely valuable here and is NOT covered by #870: the non-temporal checks.
imp_apply_field in main inspects only Date, Time and Timestamp, with default: break for
everything else, so the file's declared Int.bitWidth/is_signed, float width,
FixedSizeBinary.byteWidth and decimal precision/scale are never read on import. A bounds check
("value buffer too small for the row count") catches the narrowing cases, so this is not a
memory-safety hole — but uint64 into bigint, a wider int into a narrower column, and
decimal128(10,2) into numeric(20,4) all have matching-or-larger carriers and would be
misread silently.

That half deserves its own issue and its own PR against current main, where it conflicts with
nothing. I have not measured those three cases, only read the code, so whoever picks it up
should start by making them fail.

Also worth stating so it is not inherited as a surprise: this branch's head deleted its own
Int.is_signed check, so its uint64 into bigint arm is currently red on its own terms.
Whatever is salvaged wants rebuilding rather than rebasing.

Record of the ruling this closure rests on, which was missing here.

The owner decided the policy explicitly in session, after working through always-convert-silently, refuse-the-type, lossless-or-error, convert-and-warn, and a retry loop incrementing on UNIQUE collision. His words: "After talking through it with you, I am in agreement with your recommendation. Make it happen." The recommendation was: accept nanoseconds, narrow to microseconds, never refuse, and report how many values actually lost digits — implemented in #880.

This PR is closed as superseded by that ruling plus #870, not as incorrect. The non-temporal half of its validation is genuinely uncovered by #870 and is now tracked as #881.